Looks good overall but I added some comments to sync with the rest of the package

See also: microbiome/OMA#146

For example mia::calculateJSD have support
for both matrix and SE. Similarly, this could support TreeSE.

Support for SE should be enough for methods that do not use the tree information. Because TreeSE is SE (it inherits SE).

Currently we do not seem to have ordination methods that utilize the tree. Those exist I think but they are not commonly used. It is an interesting possibility but probably not very urgent.

So in addition to this current implementation, there could be method for TreeSE which has parameters for selecting

• dimension reduction (dimred parameter)
• number of dimensions (ncomponents)
• whether to sort the output TreeSE (sort)

Adding dimension reduction would mean that we combine the steps or ordination and sorting? Possible but at leas it would complicate the implementation because now the sorting can be done based on any ordination method output. If these are combined, we need to choose which ordination methods are supported. If I got this right.

I am not sure if the radial theta angle methods is defined beyond the 2D case, hence ncomponents might not be valid option w.r.t. neatsort.

Sorting can be also done afterwards, the added value might be limited.

However, reducedDim slot requires that number of columns/samples match. If I understood correctly, this "neatsort" can be done also for features which means that reducedDim slot cannot be utilized (scater::calculatePCA and scater::calculateMDS can still be used as they have transposed parameter and they return only the output matrix). If that is the case, then I think that we should only support method for matrix to make things simpler.

Neatsort can be done for any Nx2 numeric matrix. Usually it is applied to sample scores is samples x PCaxes score matrix but in principle other uses are possible.

TBH it might not be critical to support feature loading matrices (features x PCaxes) because this doesn't seem very common. But it is possible and potentially useful, and out current implementations seems to have that support because it can operate with arbitrary Nx2 matrices.

Indeed, we could choose to simply support sample scores and use the reducedDim slot for that. This will complicate the implementation and the added value is not yet clear. I tend to think that we could keep getNearOrder support for matrices for now. If real use cases will call for a more streamlined implementation and TreeSE support we can always add that. But then we will need to address these more complex design questions.

This is somewhat related to the question on how we should present feature loadings in general in TreeSE objects because reducedDim is indeed only supporting the sample scores, and missing the feature loadings. The metadata slot can be used for that at least.

I meant that user could first do dimension reduction with methods that add the result to reducedDim and then call getNeatOrder(). As SE does not have reducedDim slot, at least SCE should have been supported.

But I agree, that only matrix should be supported at this point. This would make the usage simple

res <- calculatePCA(tse)
order <- getNeatOrder(res)
tse <- tse[ , order]
sechm(tse)

Right, I agree: getNearOrder could support reducedDim (TreeSE/SCE). But perhaps best to think about that later since the cost/benefit ratio seems a bit open.

Thanks both for the review and comments, I'm working on all of these updates today!

So I have implemented the changes but I have a few follow up questions just before I push.

1. In my example I've used the runPCA method to create the PCA matrix. This is because it was the only method I could see in OMA - (https://microbiome.github.io/OMA/docs/devel/pages/20_beta_diversity.html#sec-other-ord-methods). Is that OK?

2. Then also when calling sechm in my example am I right that we do the PCA, get the ordering from getNeatOrder, apply that to the tse object... but when we plot with sechm - do we plot using the z-transformed data?

3. When citing the publication, does that go in @details? Or @references? Or just slipped into the main description?

4. Please also see my responses to this OMA issue - neatmap / neatsort OMA#146

5. As per Tuomas' suggestion I've simplified my centering logic. However, Tuomas suggested doing the subtractions manually, but originally I'd use the scale method. Which should I do? As I understand it the scale method can be used to do centering more precisely, and which one I choose does make a slight difference.

1. More appropriate might be calculatePCA which returns matrix directly. We could highlight better where functions come from so that user can refer to those package manuals

2. Sounds correct. Z-transformation is usually applied to heatmaps since the colors are sensitive to scales. If there are features that varies a lot compared to others in "absolute" scale, those "others" look like that they do not vary at all. When values are scaled by features, the colors are scaled by feature (and not by whole dataset). If I remember correctly sechm has scale parameter, but it might be better to do manually (plot with Z-transformed data)

3. References https://github.com/microbiome/mia/blob/devel/R/calculateUnifrac.R

4. Noted. Leo probably knows better

5. I did not know that. Use scale(), but simplify the structure. There was at least one extra if-else

Run BiocCheck::BiocCheck(). There were some NOTEs related to these lines. Resolve those notes that can reasonably do. (If line contains only URL address, you can keep that even though it exceeds 80 characters)

Resolve errors in unit test

What're the errors - there was definitely no errors when I last checked

"All checks have failed" (see above Github runs)

test_check("miaViz")
[ FAIL 1 | WARN 6 | SKIP 1 | PASS 175 ]

══ Skipped tests (1) ═══════════════════════════════════════════════════════════
• miaTime cannot be loaded (1): 'test-2plotSeries.R:5:5'

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-getNeatOrder.R:48:5'): Test check_args method ────────────────
.check_args(check_args_matrix[, c(1, 2)], "invalid_method") threw an error with unexpected message.
Expected match: "'arg' should be one of “mean”, “median”, “none”"
Actual message: "'arg' should be one of "mean", "median", "none""
Backtrace:
▆

1. ├─testthat::expect_error(...) at test-getNeatOrder.R:48:5
2. │ └─testthat:::quasi_capture(...)
3. │ ├─testthat (local) .capture(...)
4. │ │ └─base::withCallingHandlers(...)
5. │ └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
6. └─miaViz:::.check_args(check_args_matrix[, c(1, 2)], "invalid_method")
7. └─base::match.arg(centering, c("mean", "median", "none"))

[ FAIL 1 | WARN 6 | SKIP 1 | PASS 175 ]
Error: Test failures
Execution halted

3 errors ✖ | 0 warnings ✔ | 4 notes ✖
Error: Process completed with exit code 1.
Run actions/upload-artifact@v3
/usr/bin/docker exec 01b1f8ffe51deb581bc7323b620ee9945bab51cd97bc196a49f233c188b27d66 sh -c "cat /etc/*release | grep ^ID"

ok interesting, I missed that cause it passed locally with devtools::test(), but failed in the CI/CD pipeline. Anyway I've made the testing abit more robust so will commit it now, which should sort it out.

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (devel@27ca8d5). Learn more about missing BASE report.

❗ Current head 20da458 differs from pull request most recent head 404b0a8

Please upload reports for the commit 404b0a8 to get more accurate results.

@@           Coverage Diff            @@
##             devel     #128   +/-   ##
========================================
  Coverage         ?   56.62%           
========================================
  Files            ?       13           
  Lines            ?     2545           
  Branches         ?        0           
========================================
  Hits             ?     1441           
  Misses           ?     1104           
  Partials         ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Great work! Looks nice

Ubuntu check:

Error in eval(code, test_env): object 'expected_indices' not found

Mac and windows fails because they do not find new mia functions. They should be resolved automatically

Fixed the error. I also changed the allowed values for centering to be mean, median and NULL (NULL is common to denote that something has been disabled)

OK great, thanks, sounds good!